ref(tracing): rework tracing to Sentry span name/op conversion
#887
Conversation
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #887 +/- ##
==========================================
+ Coverage 73.39% 73.43% +0.04%
==========================================
Files 64 64
Lines 7423 7502 +79
==========================================
+ Hits 5448 5509 +61
- Misses 1975 1993 +18 🚀 New features to boost your workflow:
|
Corrected punctuation in the CHANGELOG regarding Sentry span overrides.
There was a problem hiding this comment.
nice, this looks a lot cleaner indeed.
I think its fair to use just the function name, from the users perspective, I think that makes sense. Although I also might want to see the fully qualified name sometimes, in particular for method names.
its a bit unfortunate that tracing does not differentiate between an impl method and a free function here.
Ideally, it should qualify methods with the impl (such as Vec::extend_from_slice).
I think not qualifying anything could lead to confusion for example if you have str::from_utf8 vs String::from_utf8, which are two different methods.
But okay, I guess that is a minor annoyance.
|
Thanks @Swatinem, that's a fair point. |
|
One thing to try would be whether it actually is the "fully qualified" name, because just concatenating the |
It seems it's just the module path, which doesn't include any information about trait impls. |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
|
Found some discussions around this topic |
|
Some still open points:
|
There was a problem hiding this comment.
Overall, I think this is nice, and seems like a reasonable change, as long as we don't expect it to be extremely disruptive to users when migrating over.
I added some small, optional comments, mostly with suggestions to improve documentation, tests, and code understandability. But overall, I think what you are doing makes sense!
JS which already has a span only tracing API uses |
|
Added default attributes in Here's how a trace looks like in Sentry: https://sentry-sdks.sentry.io/explore/traces/trace/8d4be80920b04feb03ac20aaa75f7942/?fov=0%2C697.409912109375&mode=samples&node=span-c86947b544360096&pageEnd&pageStart&project=4508694563782656&source=traces&statsPeriod=1h&table=trace×tamp=1757679523.3 |
Description
This changes how the
tracingintegration works with regards to span names and operations.(In the analysis below, I assume the spans are all created with the
tracing::instrumentmacro)Previous behavior
<module>::<function>messagefield is set, in which case: Sentry span name = tracing spanmessagemessagefield is arbitrary, not documented, and doesn't align with our conventionsmessageBehavior after this PR
tracingspan name (= function name, when using thetracing::instrumentmacro)default, as we have no way to infer anopthat makes sense - it will be hidden in the UItracingSubscriber and override values on the corresponding Sentry span:sentry.namesentry.opsentry.trace: while not related to this issue, I noticed that there is confusion on how one would achieve distributed tracing. This makes it possible without having to use the Sentry tracing API and understand the interplay between Sentry and thetracingcrate.Advantages of the change
tracing-opentelemetry, which offers aotel.namespecial propertyDrawbacks of the change
span.metadata.targetinto span attributes, like we do for logs to surface function and lineCloses
Close #880
Close #879